fix: fix pika exporter 336 version match nil and delete optional logs keyspace-stats-clock #2971
Conversation
WalkthroughThe changes in this pull request focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
buzhimingyonghu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tools/pika_exporter/exporter/metrics/parser.go (1)
55-98: Consider organizing metrics by category for better maintainability.The
EmptyRegexNameslice contains a long list of metrics that could benefit from better organization. Consider grouping related metrics using constants or separate slices by category (cache, compaction, storage, etc.) and then combining them.Here's a suggested refactor:
- v.EmptyRegexName = []string{ - "hitratio_per_sec", - "total_blob_file_size", - "block_cache_capacity", - // ... more metrics - } + // Cache metrics + cacheMetrics := []string{ + "block_cache_capacity", + "block_cache_usage", + "block_cache_pinned_usage", + } + + // Compaction metrics + compactionMetrics := []string{ + "num_running_compactions", + "compaction_pending", + "estimate_pending_compaction_bytes", + "pending_compaction_bytes_stops", + "pending_compaction_bytes_delays", + } + + // Storage metrics + storageMetrics := []string{ + "total_blob_file_size", + "live_blob_file_size", + "total_sst_files_size", + "live_sst_files_size", + "num_blob_files", + } + + // Memory metrics + memoryMetrics := []string{ + "cur_size_all_mem_tables", + "size_all_mem_tables", + "cur_size_active_mem_table", + "num_immutable_mem_table", + "num_immutable_mem_table_flushed", + } + + // Performance metrics + performanceMetrics := []string{ + "background_errors", + "num_running_flushes", + "mem_table_flush_pending", + } + + // Other metrics + otherMetrics := []string{ + "hitratio_per_sec", + "commandstats_info", + } + + // Combine all metrics + v.EmptyRegexName = append([]string{}, + cacheMetrics..., + compactionMetrics..., + storageMetrics..., + memoryMetrics..., + performanceMetrics..., + otherMetrics..., + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tools/pika_exporter/exporter/metrics/parser.go(1 hunks)tools/pika_exporter/exporter/pika.go(0 hunks)
💤 Files with no reviewable changes (1)
- tools/pika_exporter/exporter/pika.go
🔇 Additional comments (1)
tools/pika_exporter/exporter/metrics/parser.go (1)
55-98: Verify metric availability across RocksDB versions.
The addition of these RocksDB metrics to EmptyRegexName will suppress warnings when they're not found. While this aligns with the PR objective of removing optional logs, please ensure these metrics are truly optional and their absence doesn't indicate actual issues.
Let's verify the metric availability:
✅ Verification successful
Based on the shell script results, I can now provide a final response. The search revealed that these RocksDB metrics are actively used in the codebase:
- The metrics are properly defined in
src/storage/src/redis.ccusing RocksDB's official property names (e.g.,rocksdb::DB::Properties::kTotalBlobFileSize,kBlockCacheCapacity, etc.) - The metrics are consistently used across different versions (3.4.2 and 3.5.0) as shown in the test files
- The metrics are properly integrated into the monitoring dashboard (Grafana)
- The metrics have clear documentation in the code (e.g., "Memory size occupied by entries in the block cache", "The total size of all blob files across all versions")
The addition of these RocksDB metrics to EmptyRegexName is appropriate and safe.
These metrics are properly integrated into the codebase and their absence in certain scenarios is expected, not problematic. The metrics are used for monitoring purposes and their suppression aligns with the PR's objective of removing optional logs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for RocksDB metric usage and documentation
# to verify their availability across versions
# Search for metric usage in the codebase
echo "Searching for metric usage..."
for metric in "total_blob_file_size" "block_cache_capacity" "background_errors" "num_running_flushes" "block_cache_usage" "compaction_pending"; do
echo "=== $metric ==="
rg -A 2 "$metric" || true
done
# Search for metric documentation or version requirements
echo "Searching for documentation..."
fd -t f "README|METRICS|DOC" -X rg "RocksDB.*metrics|metrics.*RocksDB"
Length of output: 46580
delete pika_3.3.6 not found
Delete optional logs
Summary by CodeRabbit
New Features
VersionChecker336with additional regex names for empty checks, improving metric validation.Bug Fixes
statsKeySpacefunction to streamline execution flow.